Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Windows: add IconExtWindows::from_resource_name #4137

Merged
merged 4 commits into from
Feb 26, 2025
Merged

Windows: add IconExtWindows::from_resource_name #4137

merged 4 commits into from
Feb 26, 2025

Conversation

rctlmk
Copy link
Contributor

@rctlmk rctlmk commented Feb 23, 2025

Adds a new method, IconExtWindows::from_resource_name, on Windows. Works pretty much the same as IconExtWindows::from_resource.

  • Tested on all platforms changed
  • Added an entry to the changelog module if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior

@rctlmk rctlmk requested a review from notgull as a code owner February 23, 2025 22:17
@kchibisov
Copy link
Member

Do we really need this, given that other API can be used as well?

madsmtm
madsmtm reviewed Feb 24, 2025
View reviewed changes
Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should instead change from_resource to take a string instead of the ordinal ID?

@madsmtm madsmtm added DS - windows S - api Design and usability labels Feb 24, 2025
@rctlmk
Copy link
Contributor Author

rctlmk commented Feb 24, 2025

Since there's this resource pointer conversion thing, it would be nice to have both u16 and &str methods, I think.
Basically, if the resource file looks something like this:

app ICON "a.ico"
1 ICON "b.ico"

Icon::from_resource_name("app", None) and Icon::from_resource_id(1, None) will work, but Icon::from_resource_name("1", None) won't.

@madsmtm
Copy link
Member

madsmtm commented Feb 24, 2025

Ah, makes sense. Mind documenting this too? And e.g. link to https://learn.microsoft.com/en-us/windows/win32/menurc/icon-resource ?

@rctlmk
Copy link
Contributor Author

rctlmk commented Feb 25, 2025

Yes, good thinking.

@rctlmk rctlmk force-pushed the icon-from-res-name branch from a2ad299 to 45c78a6 Compare February 25, 2025 22:31
madsmtm
madsmtm approved these changes Feb 26, 2025
View reviewed changes
Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is perfect

@madsmtm madsmtm merged commit 4687942 into rust-windowing:master Feb 26, 2025
57 checks passed
@madsmtm madsmtm added this to the Version 0.30.10 milestone Feb 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@madsmtm madsmtm madsmtm approved these changes

@notgull notgull Awaiting requested review from notgull notgull is a code owner

Assignees
No one assigned
Labels
DS - windows S - api Design and usability
Milestone
Version 0.30.10
Development

Successfully merging this pull request may close these issues.
3 participants
@rctlmk @kchibisov @madsmtm